Skip to content

Fix sentinel connections #3376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix sentinel connections #3376

wants to merge 3 commits into from

Conversation

garionphx
Copy link

@garionphx garionphx commented Sep 10, 2024 •
edited
Loading

When passing args to Redis(), using the connection_kwargs args allows us to use authentication and other connection features.

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Modified the arguments passed to Redis() when forming Sentinel connections

John Sutherland added 2 commits September 9, 2024 20:13
Fix sentinel connections
a1dbf59 
When passing args to Redis(), using the connection_kwargs args allows us to use authentication and other connection features.
Update CHANGES
751448d
@garionphx garionphx mentioned this pull request Sep 10, 2024
Open
@garry-t
Copy link

garry-t commented Sep 10, 2024

great, thank you!

@vladvildanov vladvildanov added the breakingchange API or Breaking Change label Sep 12, 2024
@vladvildanov
Copy link
Collaborator

@garionphx Thanks for your contribution! Make sure that tests passes

@vladvildanov
Copy link
Collaborator

@garionphx Since this changes makes self.sentinel_kwargs argument completely redundant, it should be removed from constructor properties and it will be a breaking change.

vladvildanov
vladvildanov reviewed Sep 12, 2024
View reviewed changes
redis/sentinel.py
@@ -240,7 +240,7 @@ def __init__(
self.sentinel_kwargs = sentinel_kwargs

self.sentinels = [
Redis(hostname, port, **self.sentinel_kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is the only place where self.sentinel_kwargs are used we have to remove it

@rad-pat
Copy link

rad-pat commented Oct 15, 2024

This doesn't seem correct. The sentinel_kwargs are the connection_kwargs passed in to make the sentinel connections and the connection_kwargs are those used for making Redis connections. The arguments can be different for each and should be allowed to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladvildanov vladvildanov vladvildanov left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
breakingchange API or Breaking Change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@garionphx @garry-t @vladvildanov @rad-pat @petyaslavova